-
Notifications
You must be signed in to change notification settings - Fork 721
Solver: shorten the skipping message if needed #11062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
A trivial project that depends on aeson but requires a version more recent that the latest, built with existing The version of |
70aef9e to
1fdf18e
Compare
| | x@(POption i linkedTo) <- xs | ||
| ]) | ||
| | x@(POption i linkedTo) <- take 3 xs | ||
| ] ++ if length xs >= 3 then " and earlier versions" else "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use a number less that 3 here, a number of the UnitTest tests (4 to 6 from memory) will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when length of xs is exactly 3, cabal will print the entirety of the list but still says "and earlier versions"?..
3 looks like too many still. I'd suggest print 1 and fix the unit tests.
009e43f to
d961f94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments:
- I think that -v3 should continue to show all of the versions, for debugging or when users need to see which versions cabal considered or their order. Then -v3 could also be used by any existing unit tests that would be broken by this change.
- It looks like this change would shorten the skipping/rejecting message even when the solver doesn't reject all of the versions. Is that intentional, or did you mean to implement something more like #10926? Another idea is to shorten the list when the solver rejects/skips all of the remaining versions (from the current version to the least preferred version).
- It might be better to continue listing any installed or linked versions, no matter where they appear in the list.
- I agree with @ulysses4ever that only one version should be listed, at least when they are all non-linked source versions.
- There should also be a test for shortening the "rejecting" message.
- It would help to have a test where the solver rejects or skips multiple versions for one reason and then rejects another version for another reason. This test case would address (2).
cabal-install-solver/src/Distribution/Solver/Modular/Message.hs
Outdated
Show resolved
Hide resolved
I have this in my working tree.
Was not even aware that my change would also affect successful solutions. IMO, it should only affect the output when there is no successful solution. I will need some pointers to move forward on this.
Sorry, I do not even understand this comment.
Fixed in my local version already. Already have the full list being printed when
Any idea what the unit test for this would look like? I tried to add a test to
Clues? |
d961f94 to
f8b233f
Compare
|
Obviously these commtis should all be rebased down to a single commit. I am keeping this separate for now as each commit addresses a separate review comment. |
|
That's what |
|
I know that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that this PR is already a standalone improvement to the rejecting message, so I think it would be better to merge it before implementing the other parts of #10926 that I mentioned in my previous comment.
There should also be a test for shortening the "rejecting" message.
Any idea what the unit test for this would look like? I tried to add a test to
cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hsbut I cannot trigger it.
You could use a global constraint (equivalent to the --constraint flag) instead of a build-depends constraint, since global constraints haven't been optimized, and each version needs to be rejected separately.
I haven't reviewed the new changes yet, and I'm busy this week, so I probably won't have time for another review until later next week.
cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs
Outdated
Show resolved
Hide resolved
c3f07d0 to
d60581d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the rest of the code, and I only have a few small comments to add to my last review.
cabal-install-solver/src/Distribution/Solver/Modular/Message.hs
Outdated
Show resolved
Hide resolved
f23628b to
01873d1
Compare
78870a1 to
de0d451
Compare
7a99c02 to
407bd68
Compare
|
@erikd Is this ready for review again? |
810714f to
7e081a4
Compare
7e081a4 to
fe49f12
Compare
dc508a1 to
a8bbb3b
Compare
Also add a test to prevent this regressing. Closes: haskell#4251
Use `setVerbose` in two of the unit tests in preparation for upcoming changes to the `skipping ...` message.
And fix associated unit test expected string.
a8bbb3b to
c9b5ca3
Compare
Also add a test to prevent this regressing.
Closes: #4251
Please read Github PR Conventions and then fill in one of these two templates.
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.